Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[DEPENDENCY] Switch from "rimraf" to native "fs.rm" #1098

Merged
merged 1 commit into from
Nov 25, 2024
Merged

[DEPENDENCY] Switch from "rimraf" to native "fs.rm" #1098

merged 1 commit into from
Nov 25, 2024
+84 −40

Conversation

matz3
Copy link
Member

@matz3 matz3 commented Nov 13, 2024

No description provided.

@coveralls
Copy link

coveralls commented Nov 13, 2024 •
edited
Loading

Coverage Status

coverage: 94.907% (+0.002%) from 94.905%
when pulling 58a1a77 on use-native-fs-rm-recursive-force
into 895fb41 on main.

codeworrior
codeworrior previously approved these changes Nov 13, 2024
View reviewed changes
Copy link
Member

@codeworrior codeworrior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me a while to understand suffix "p" in "mkdirp", but "rmrf" finally explained it :-)

@matz3
Copy link
Member Author

matz3 commented Nov 14, 2024

As rimraf still uses its own implementation on Windows, I want to do some manual testing before merging this. Our unit tests don't cover this part well as many times the FS APIs are stubbed.

@matz3 matz3 force-pushed the use-native-fs-rm-recursive-force branch from f03dd28 to 58a1a77 Compare November 25, 2024 14:28
@matz3 matz3 marked this pull request as ready for review November 25, 2024 14:30
@matz3 matz3 mentioned this pull request Nov 25, 2024
Merged
@matz3
Copy link
Member Author

matz3 commented Nov 25, 2024

I've tested this on Windows via ui5 build --all --clean-dest and ui5 build jsdoc --all --clean-dest and didn't see any issues. So I'm eager to merge this.

Also see SAP/ui5-project#780

@matz3 matz3 changed the title [FEATURE] Switch from "rimraf" to native "fs.rm" [DEPENDENCY] Switch from "rimraf" to native "fs.rm" Nov 25, 2024
@matz3 matz3 requested a review from a team November 25, 2024 14:38
codeworrior
codeworrior approved these changes Nov 25, 2024
View reviewed changes
Copy link
Member

@codeworrior codeworrior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@matz3 matz3 merged commit 8ddadf4 into main Nov 25, 2024
17 checks passed
@matz3 matz3 deleted the use-native-fs-rm-recursive-force branch November 25, 2024 15:26
matz3 added a commit that referenced this pull request Nov 29, 2024
@matz3
[DEPENDENCY] Switch from "rimraf" to native "fs.rm" (#1098)
0adf4da
@matz3
Copy link
Member Author

matz3 commented Nov 29, 2024

I forgot to adjust the commit message via squash merge. The merged commit on main has been adjusted as 0adf4da.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeworrior codeworrior codeworrior approved these changes

@RandomByte RandomByte RandomByte approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@matz3 @coveralls @RandomByte @codeworrior